[v12] feat(ui-range-input): rework RangeInput #2318
Conversation
ToMESSKa
changed the title
|
| const thumbPosition = { | ||
| deprecated: { | ||
| marginTop: `calc(-1 * ${componentTheme.handleSize} / 4)` | ||
| marginTop: `calc(-1 * ${componentTheme.handleSize} / 4 - 1px)` |
There was a problem hiding this comment.
because the track got a '1px' wide border, the thumb position changed too so I adjusted the missing pixels here,
There was a problem hiding this comment.
this feels a bit magic to me, maybe you could create a const for the borderwidth and use that above and here, also a comment like this would be good in the code
ToMESSKa
changed the title
ToMESSKa
force-pushed
the
INSTUI-4805-range-input-rework
branch
from
5b5c44f to
de126e9
Compare
adamlobler
left a comment
adamlobler
left a comment
There was a problem hiding this comment.
On the original component the padding around the value tooltip was only a horizontal padding, now its a full padding, we should revert this to the original one:
Other than that, it looks okay. There’s just one more small thing, but we should update the tokens as well since we don’t have a proper solution right now... The focus ring isn’t visible in the dark theme, and we don’t currently have a focus color that fits here. I’ll update the tokens and get back to you with a solution:
ToMESSKa
force-pushed
the
INSTUI-4805-range-input-rework
branch
from
de126e9 to
21f414b
Compare
@adamlobler Now the padding token only applies the horizontal padding, I set the vertical padding to 0. Also the focus ring should be okay now, can you check it please? |
adamlobler
left a comment
adamlobler
left a comment
There was a problem hiding this comment.
The paddings and the focusBorder looks okay, thanks for the modification👌 We just need to make some smaller modifications on the token values but its not on your side.
ToMESSKa
force-pushed
the
INSTUI-4805-range-input-rework
branch
from
21f414b to
5ef95fd
Compare
| function boxShadowToCSSString(boxShadowObject: TokenBoxshadowValueInst) { | ||
| // weird string concatenation is to make it look nice in the debugger |
There was a problem hiding this comment.
that one is merged now, please check if you have any conflicts
| small: { | ||
| fontSize: componentTheme.valueSmallFontSize, | ||
| padding: componentTheme.valueSmallPadding, | ||
| padding: `${'0 ' + componentTheme.valueSmallPadding}`, |
There was a problem hiding this comment.
if you only want to set the right and left paddings, a better approach would be to use the paddingInline css prop so you don't have to set the top and bottom to 0
| const thumbPosition = { | ||
| deprecated: { | ||
| marginTop: `calc(-1 * ${componentTheme.handleSize} / 4)` | ||
| marginTop: `calc(-1 * ${componentTheme.handleSize} / 4 - 1px)` |
There was a problem hiding this comment.
this feels a bit magic to me, maybe you could create a const for the borderwidth and use that above and here, also a comment like this would be good in the code
| function boxShadowToCSSString(boxShadowObject: TokenBoxshadowValueInst) { | ||
| // weird string concatenation is to make it look nice in the debugger |
There was a problem hiding this comment.
that one is merged now, please check if you have any conflicts
INSTUI-4805
ISSUE:
TEST PLAN: